Repository navigation
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
|
All contributors have signed the CLA ✍️ ✅ |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
20f4f28 to
7eca905
Compare
|
@shawnborton @QichenZhu One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
Let us know when this is ready for @Expensify/design review. |
…ased callback names.
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@QichenZhu Could you take another look? I believe I’ve addressed all of your comments. Thank you. |
|
Precondition: Have a USD workspace Steps:
Expected result: the converted amount shows zero. Actual result: the converted amount shows the original non-zero amount. nab.movIt's not a blocker for this PR, but I think it's a bug worth fixing separately. |
| type ColumnItem = { | ||
| /** Localized column label, also used as the screen reader name. */ | ||
| text: string; | ||
|
|
||
| /** Column ID passed back when a row is reordered. */ | ||
| value: SearchCustomColumnIds; | ||
|
|
||
| /** Stable row key matching the column ID. */ | ||
| keyForList: SearchCustomColumnIds; | ||
|
|
||
| /** Selected columns appear before the alphabetically ordered unselected columns. */ | ||
| isSelected: boolean; | ||
|
|
||
| /** Required fields cannot be deselected, but can still be reordered. */ | ||
| isDisabled: boolean; | ||
|
|
||
| /** Unselected columns remain visible without becoming drop targets. */ | ||
| isDragDisabled: boolean; | ||
|
|
||
| /** Drag icon wrapped in a touch target by the native variant. */ | ||
| leftElement: React.JSX.Element; | ||
| }; | ||
|
|
||
| type ColumnsSettingsListItemProps = Pick<DraggableListRenderItemParams<ColumnItem>, 'item' | 'isFocused' | 'drag' | 'isActive'> & { | ||
| /** Toggles optional columns without changing the selection of required columns. */ | ||
| onSelectRow: ListItemProps<ColumnItem>['onSelectRow']; | ||
|
|
||
| /** Omitted when this row is already first or cannot be reordered. */ | ||
| onMoveUp?: () => void; | ||
|
|
||
| /** Omitted when the next row is unselected or this row cannot be reordered. */ | ||
| onMoveDown?: () => void; | ||
| }; |
There was a problem hiding this comment.
The AI coder understands that comments should add information, which is good, but it misunderstands "information" and puts implementation details into JSDoc.
For example, "Drag icon wrapped in a touch target by the native variant" says which component wraps the icon. It will change over time, and it doesn't explain the prop's purpose or which part of the UI it belongs to. The comment before the refactor was better: "Element rendered on the left side of the list item (e.g. drag handle)".
Another example is "Selected columns appear before the alphabetically ordered unselected columns". It's a detail of only one screen. The comment before the refactor was better since it explained the prop's purpose: the table shows only the selected columns.
App/src/components/ColumnsSettingsList.tsx
Lines 30 to 76 in 3a7443e
| expect(category).toContainElement(hint); | ||
| for (let ancestor = hint.parent; ancestor && ancestor !== category; ancestor = ancestor.parent) { | ||
| expect(ancestor.type).not.toBe(MenuItem.Row); | ||
| expect(ancestor.type).not.toBe(MenuItem.Trailing); | ||
| } |
There was a problem hiding this comment.
I'm not sure we need to test the component hierarchy.
| > | ||
| {title} | ||
| </Text> | ||
| {!!shouldShowPaid && <Icon src={icons.Checkmark} />} |
| case CONST.SEARCH.TABLE_COLUMNS.HELD: | ||
| value = translate(isOnHold(transaction) ? 'common.yes' : 'common.no'); | ||
| break; |
|
@MelvinBot review this again, including accessibility issues |
|
@codex review |
Re-review at
|
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
Addressing the comments. Thanks for the review. |
|
|
|
back to you @QichenZhu |
70740e2 to
e6a5e1e
Compare
Wait, just a sec, finishing up a few details that were left behind. I’ll let you know when it's ready to continue with the review. Thanks. |
|
This PR adds a new |
|
The PR turned out a bit larger than I had anticipated; I suspect I might be touching areas that don't actually need changes. I’m reviewing it carefully and will let you know as soon as it’s ready for review. Thanks. |



Explanation of Change
This PR displays configured report columns as read-only fields in workspace expense and single-expense views, while preserving existing expense editors and required fields.
Customize fields reuses the column picker with a separate expense-field preference. Changing expense fields no longer rewrites the report table's columns. Existing table preferences seed additional fields until the expense selection is customized. The picker supports reorder, reset, native accessibility actions and feedback, and grouped native scrolling.
Fixed Issues
$ #101300
PROPOSAL: N/A
Tests
Test Steps
Current revision validation
Follow-up: fixed the alphabetical export-key ordering; ExportOnyxState and field-preference suites pass locally (60 tests). Reused useReportTransactions instead of filtering the full transaction collection in the picker. All three updated commits have verified GPG signatures on GitHub.
Automated: 410 passing tests across 17 suites, including preference rollback, late-arriving preferences/workspace data, navigation, required fields, keyboard reorder, nested scrolling and HTML selection.
TypeScript, React Compiler, formatting and spelling for the PR files pass. Lint for the PR files passes.
Interactive: Chrome and iPhone Simulator against staging; save/reopen/cancel/reset, extra-field display, required Category, native drag and simple-list autoscroll, grouped web keyboard reorder and table order verified.
Web mouse drag remains inconclusive in automation. Grouped native scrolling on a wide device has not been interactively verified. Android was excluded due to the local SDK setup; physical-device VoiceOver testing was waived by the author.
Verify that no errors appear in the JS console
Offline tests
Same as Test Steps
QA Steps
Same as Test Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Screen.Recording.2026-10-06.at.10.33.27.mov
Android: mWeb Chrome
iOS: Native
102457_ios_native.mov
iOS: mWeb Safari
MacOS: Chrome / Safari
102457_web_chrome.mov